-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Per Particle Logger #4359
Per Particle Logger #4359
Conversation
better doc re: input class copy constructors remove InputSection changes
d47c69d
to
5a34c19
Compare
src/QMCDrivers/Crowd.h
Outdated
@@ -104,6 +108,8 @@ class Crowd | |||
const MultiWalkerDispatchers& dispatchers_; | |||
|
|||
private: | |||
/// For some output purposes its useful if a crowd has an identifier for it on a rank. | |||
const int crowd_id_ = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you point me a use case? Such state usually concerns me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reconsidering this state, I thought it was necessary but now I think it is just lazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to eliminate the crowd_id_ and simply take the Walker.ID seriously. This is a more likely desired for a per particle log anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waker.ID is as bad as crowd_id_. It was not taken seriously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it requires less disturbance to the code. To log per particle per walker hamiltonian components there must be some identification of the walkers. I added unit testing of the walker ids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
walker ID is global (across all MPI ranks), very hard to keep track of. You can create some local data structure for the indexing, it is far better than replying on an global ID.
I would like to break the symmetry here and get this activity moving again, not only this PR. Peter and I discussed this last week. First of all, since this PR doesn't break existing functionality, the general guidance about code moving in a good direction being mergeable applies. Second, one of the use cases for this level of logging is to help diagnose a failed run, at least on "replay" using the same seed and run configuration. In this case having a unique ID for each walker would be very useful, so this is what we should work towards. There is no question that we would have used the capability for this purpose several times in the last year. In VMC the IDs are trivial, since the walker count is constant. In DMC we need e.g. an incrementing counter and to make sure we can count 100s millions of walkers. Since the walker creation/destruction logic is global this is also not difficult. A new ID is only needed when new walkers are created. The required state information is a single (long?) integer. This can be worked towards in subsequent PRs. As with general use of the per particle logger, we are concerned with functionality and achievable insight, not straight line speed. |
@@ -48,6 +48,9 @@ class StructFact : public QMCTraits | |||
/** Constructor - copy ParticleSet and init. k-shells | |||
* @param lattice long range box | |||
* @param kc cutoff for k | |||
* | |||
* At least in the batched version Structure factor is _NOT_ valid | |||
* after construction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you are trying to describe an issue. Could you put it as an issue in github?
I think we can make this PR moving with minor changes.
Indexing is unavoidable when recording into a big buffer/file. What I'm strongly against is to keep it as a state of walker. Indexing should be short lived whenever possible. Each element should avoid remembering its index, only the object handling a set of elements deal with indexing in restricted scopes.
Creating new ID means the ID of other walkers can be out-dated. This requires global synchronization.
|
The ideas is that walkers have unique IDs for their entire lifetime. |
I still need an answer for how walker id is used for in this PR and why this is necessity. I don't mean to block this PR on this but the current use needs to be documented. How do we want that in the future will be a separate topic. |
Fair point Ye. For future walker ID/numbering discussions: if we don't mind not having them numbered consecutively (I don't), we could just number them (walker_generation*no_mpi_tasks+my_mpi_task_no); this needs no coordination at all since the IDs can be generated at the mpi task level (I assume we will take all these decisions at this level - I don't think anything is needed at the crowd level). Then the only state needed is at the task level. This then leaves the question as to whether we want to save it for restarts... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: there is still no documentation about walker ID in this PR.
Test this please |
test this please |
Test this please |
Proposed changes
This PR adds per particle Hamiltonian reporting to estimators through the entire application and provides the proof of concept particle Hamiltonian logger. The logger is not optimized for performance nor is the output optimal for actual use. If anyone would like to improve it further they should feel free.
More complete documentation of the Estimator architecture will be added in future PR's.
What type(s) of changes does this code introduce?
Delete the items that do not apply
Does this introduce a breaking change?
What systems has this change been tested on?
LECONTE
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.